Feature/1.5.8/zenoh utransport impl #65

gregmedd · 2024-07-08T18:21:17Z

With patches applied to #54

gregmedd

Stopping part way into this review. In addition to the comments recorded, the known list of TODO items includes:

Rework invoke_nonblock_callback call
Finish all TODO in PR
Implement cleanupListener
Finish and add tests for attachmentToUAttributes/uattributesToAttachment
Implement this for C++ https://github.com/eclipse-uprotocol/up-spec/pull/188/files
Add all necessary tests
Switch to zenoh 1.0

gregmedd · 2024-07-08T18:27:30Z

include/up-transport-zenoh-cpp/ZenohUTransport.h

+#define ZENOHCXX_ZENOHC
+#include <zenoh.hxx>
+
+namespace zenohc {
+
+class OwnedQuery {
+public:
+	OwnedQuery(const z_query_t& query) : _query(z_query_clone(&query)) {}
+
+	OwnedQuery(const OwnedQuery&) = delete;
+	OwnedQuery& operator=(const OwnedQuery&) = delete;
+
+	~OwnedQuery() { z_drop(&_query); }
+
+	Query loan() const { return z_loan(_query); }
+	bool check() const { return z_check(_query); }
+
+private:
+	z_owned_query_t _query;
+};
+
+using OwnedQueryPtr = std::shared_ptr<OwnedQuery>;
+
+}  // namespace zenohc


This should be relocated into the .cpp file. It's a detail specific to this implementation and does not need to be exposed externally.

It is temporary workaround and it should be removed after switching to zenohcpp 1.0

gregmedd · 2024-07-08T18:32:42Z

include/up-transport-zenoh-cpp/ZenohUTransport.h

+		    : listener(listener), zenoh_key(zenoh_key) {}
+
+		bool operator==(const ListenerKey& other) const {
+			return listener == other.listener && zenoh_key == other.zenoh_key;


Suggested change

return listener == other.listener && zenoh_key == other.zenoh_key;

return (listener == other.listener) && (zenoh_key == other.zenoh_key);

This && might need to be changed or an additional equality operator for CallableConn only might need to be added, assuming the intent is for this key to be used as part of the callback cleanup process.

Or maybe that's not the intent here - the operator< sorts on both

This class needs only if the cleanupListener will receive sink/source filters like in rust, but currently it receives only listener, and I still haven't received an answer why this is so.

gregmedd · 2024-07-08T18:37:39Z

include/up-transport-zenoh-cpp/ZenohUTransport.h

+	using RpcCallbackMap = std::map<UuriKey, CallableConn>;
+	using SubscriberMap = std::map<ListenerKey, zenoh::Subscriber>;
+	using QueryableMap = std::map<ListenerKey, zenoh::Queryable>;
+	using QueryMap = std::map<std::string, zenoh::OwnedQueryPtr>;


Some clarification on what these are would be helpful.

The types are also only used once and not particularly long, so the using statements could probably be removed.

gregmedd · 2024-07-08T18:40:20Z

include/up-transport-zenoh-cpp/ZenohUTransport.h

+	static std::string toZenohKeyString(
+	    const std::string& default_authority_name, const v1::UUri& source,
+	    const std::optional<v1::UUri>& sink);


Should be documented.

gregmedd · 2024-07-08T18:41:43Z

include/up-transport-zenoh-cpp/ZenohUTransport.h

+	static v1::UStatus uError(v1::UCode code, std::string_view message);
+
+	static std::vector<std::pair<std::string, std::string>>
+	uattributesToAttachment(const v1::UAttributes& attributes);
+
+	static v1::UAttributes attachmentToUAttributes(
+	    const zenoh::AttachmentView& attachment);
+
+	static zenoh::Priority mapZenohPriority(v1::UPriority upriority);
+
+	static v1::UMessage sampleToUMessage(const zenoh::Sample& sample);


Could probably be in the anonymous namespace of the .cpp file.

gregmedd · 2024-07-08T19:07:44Z

src/ZenohUTransport.cpp

+	// TODO: more efficient way?
+	res.ParseFromString(std::string(attachment_vec[1].as_string_view()));


Improvements should be tracked as issues.

src/ZenohUTransport.cpp

gregmedd · 2024-07-08T19:17:11Z

src/ZenohUTransport.cpp

+
+UMessage ZenohUTransport::sampleToUMessage(const Sample& sample) {
+	UAttributes attributes;
+	if (sample.get_attachment().check()) {


What is check()?

If the check fails, the UMessage would have blank attributes. That doesn't seem to be valid. The return type should probably be Expected<UMessage, ...> so errors can be returned.

check() means that the attachment exists in the sample

gregmedd · 2024-07-08T19:23:47Z

src/ZenohUTransport.cpp

+	message.set_payload(payload);
+	message.set_allocated_attributes(&attributes);


set_allocated_attributes is not safe. Use assignment on the dereferenced mutable attributes:

Suggested change

message.set_payload(payload);

message.set_allocated_attributes(&attributes);

message.set_payload(std::move(payload));

(*message.mutable_attributes()) = std::move(attributes);

Yes, it was already fixed in my PR

gregmedd · 2024-07-08T19:29:06Z

src/ZenohUTransport.cpp

+      session_(expect<Session>(open(
+          std::move(expect(config_from_file(configFile.string().c_str())))))) {}


This would be easier to read as a function in the anonymous namespace.

Suggested change

session_(expect<Session>(open(

std::move(expect(config_from_file(configFile.string().c_str())))))) {}

session_(openSession(configFile)) {}

How is the error condition for the expect<Session> checked? That error should probably be an exception.

"expect" raises the exception

Most of the basic functionality is present, but there are still several TODO items remaining: * Rework invoke_nonblock_callback call * Finish all TODO comments in code files * Implement cleanupListener * Add and finish tests * Fix anything that fails tests * Switch to Zenoh 1.0 when available

* Remove use to gtest outside of test tree. * Address error message from zenohcpp library. * Switch to our temporary zenohcpp conan recipe.

gregmedd · 2024-07-25T00:58:12Z

This is an early draft of the zenoh utransport implementation. It is known to contain bugs and not be fully functional.

We plan on merging this once eclipse-uprotocol/up-cpp#240 has been merged. All existing feedback will be captured as bugs to be addressed after the merge. This will allow multiple contributors to proceed in parallel using this code as a foundation.

debruce · 2024-07-25T15:45:45Z

src/ZenohUTransport.cpp

+			attributes = attachmentToUAttributes(query.get_attachment());
+		}
+		auto id_str = serializer::uuid::AsString().serialize(attributes.id());
+		std::unique_lock<std::mutex> lock(query_map_mutex_);


Wouldn't it be better to take the data structs that need thread safety, and turn them into classes holding the map or whatever and the mutex, and having accesor methods to do the locking? This style would make the main code much more readable.

Make sense, but before we should solve the question about cleanupListener signature, because if it will be as is (but not as rust one) we should rework these internals types.

@sashacmc - I'll summarize what we discussed on cleanupListener over slack here so its available for everyone.

cleanupListener() is not a direct equivalent to unregisterListener() in other language libraries. Since we are using connection objects to represent listener callbacks, by the time cleanupListener() is called the connection is already broken. Calling a disconnected handle from the transport side will do nothing.

The purpose of cleanupListener() is purely informational - it exists to inform the transport implementation that a connection to a listener / callback function has been broken and that, depending on the details of the transport's implementation, it may need to schedule a cleanup operation. Since the connection to the callback function is already broken, this cleanup does not need to occur immediately. It can be deferred if that makes the cleanup easier.

Additionally, while the listener handle parameter passed to cleanupListener() can be compared against other listener handles to find matching handles, that is not strictly necessary. The CallerHandle class used there has both an isConnected() method and a boolean conversion operator that will return false if the handle is no longer connected. A transport implementation could just remove all unconnected handles after cleanupListener() is called.

debruce · 2024-07-25T15:55:23Z

src/ZenohUTransport.cpp

+		if (auto resp_callback_it = rpc_callback_map_.find(source_str);
+		    resp_callback_it == rpc_callback_map_.end()) {
+			return uError(UCode::UNAVAILABLE, "failed to find UUID");
+		} else {


In relation to my prior comment about making a collection of thread safe data struct wrappers, this code shows the error object being created inside a locked region when it really shouldn't be. It seems better to me to have a thread-safe RpcCallbackMap class with an accessor like "[[nodiscard]] bool find(CallableConn& thing_to_capture)".

debruce · 2024-07-25T15:57:33Z

src/ZenohUTransport.cpp

+	std::string data;
+	attributes.SerializeToString(&data);
+
+	res.push_back(std::make_pair("", version));


Should this be emplace_back instead of push_back?

Updating up-cpp version, rewriting pub/sub test to use L2 APIs, updating the transport to use the revised up-cpp interface from up-cpp#240, and fixing some general bugs. Also adds a zenoh config json for use with the tests.

The pre-release up-cpp has critical bugfixes required for this code to build. Also makes sure the test configs are captured in the build artifacts so tests can run in CI.

We know that the current state of this implementaiton does not fully work. Making this change is a compromise - it keeps the tests active and running so we can see the output as we work on resolving issues, but does not prevent us from merging early, incomplete code. Once this implementation is more stable, we will re-enable this check

Fix unittest and code cleanup: * fix some notes from #65 * implement cleanupListener * change listener storing key * fix lambda transmission to zenoh

gregmedd commented Jul 8, 2024

View reviewed changes

gregmedd force-pushed the feature/1.5.8/zenoh-utransport-impl branch from c53133f to 3975338 Compare July 8, 2024 20:47

Patch draft implementation to get it building

0b2e638

* Remove use to gtest outside of test tree. * Address error message from zenohcpp library. * Switch to our temporary zenohcpp conan recipe.

gregmedd force-pushed the feature/1.5.8/zenoh-utransport-impl branch from 3975338 to cefbca4 Compare July 25, 2024 00:50

gregmedd marked this pull request as ready for review July 25, 2024 00:54

gregmedd mentioned this pull request Jul 25, 2024

Adding temporary up-cpp 1.0.1-rc0 eclipse-uprotocol/up-conan-recipes#23

Merged

debruce reviewed Jul 25, 2024

View reviewed changes

gregmedd added 2 commits July 26, 2024 12:00

Updating ZenohUTransport with revised API

7388a7e

Updating up-cpp version, rewriting pub/sub test to use L2 APIs, updating the transport to use the revised up-cpp interface from up-cpp#240, and fixing some general bugs. Also adds a zenoh config json for use with the tests.

Minor revision to README with updated package numbers

f82c1d1

gregmedd force-pushed the feature/1.5.8/zenoh-utransport-impl branch from 057e129 to dab0902 Compare July 26, 2024 19:02

gregmedd self-assigned this Jul 26, 2024

gregmedd force-pushed the feature/1.5.8/zenoh-utransport-impl branch 2 times, most recently from 2cc0f8b to 78de5e5 Compare July 27, 2024 01:42

gregmedd added 2 commits July 26, 2024 18:46

Updating CI and pointing at pre-release up-cpp

ab53e69

The pre-release up-cpp has critical bugfixes required for this code to build. Also makes sure the test configs are captured in the build artifacts so tests can run in CI.

gregmedd force-pushed the feature/1.5.8/zenoh-utransport-impl branch from 78de5e5 to db76fd6 Compare July 27, 2024 01:46

gregmedd merged commit b701e35 into eclipse-uprotocol:main Jul 27, 2024
4 of 5 checks passed

gregmedd deleted the feature/1.5.8/zenoh-utransport-impl branch July 27, 2024 01:52

gregmedd linked an issue Jul 29, 2024 that may be closed by this pull request

Write base set of extra PublisherSubscriberTest for up-transport-zenoh-cpp #58

Closed

sashacmc mentioned this pull request Jul 29, 2024

Fix unittest and code cleanup #69

Merged

gregmedd added a commit that referenced this pull request Jul 29, 2024

Merge pull request #69 from sashacmc/fix_test

9499a4c

Fix unittest and code cleanup: * fix some notes from #65 * implement cleanupListener * change listener storing key * fix lambda transmission to zenoh

gregmedd mentioned this pull request Jul 30, 2024

Write base set of extra PublisherSubscriberTest for up-transport-zenoh-cpp #58

Closed

gregmedd linked an issue Jul 30, 2024 that may be closed by this pull request

Implement and test transport::ZenohUTransport #51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/1.5.8/zenoh utransport impl #65

Feature/1.5.8/zenoh utransport impl #65

gregmedd commented Jul 8, 2024

gregmedd left a comment

gregmedd Jul 8, 2024

sashacmc Jul 25, 2024

gregmedd Jul 8, 2024

gregmedd Jul 8, 2024

sashacmc Jul 25, 2024

gregmedd Jul 8, 2024

gregmedd Jul 8, 2024

gregmedd Jul 8, 2024

gregmedd Jul 8, 2024

gregmedd Jul 8, 2024

gregmedd Jul 8, 2024

sashacmc Jul 25, 2024

gregmedd Jul 8, 2024

sashacmc Jul 25, 2024

gregmedd Jul 8, 2024

gregmedd Jul 8, 2024

sashacmc Jul 25, 2024

gregmedd commented Jul 25, 2024 •

edited

Loading

debruce Jul 25, 2024

sashacmc Jul 25, 2024

gregmedd Jul 26, 2024

debruce Jul 25, 2024

debruce Jul 25, 2024

sashacmc Jul 25, 2024

	return listener == other.listener && zenoh_key == other.zenoh_key;
	return (listener == other.listener) && (zenoh_key == other.zenoh_key);

		// TODO: more efficient way?
		res.ParseFromString(std::string(attachment_vec[1].as_string_view()));

		message.set_payload(payload);
		message.set_allocated_attributes(&attributes);

		session_(expect<Session>(open(
		std::move(expect(config_from_file(configFile.string().c_str())))))) {}

	session_(expect<Session>(open(
	std::move(expect(config_from_file(configFile.string().c_str())))))) {}
	session_(openSession(configFile)) {}

Feature/1.5.8/zenoh utransport impl #65

Feature/1.5.8/zenoh utransport impl #65

Conversation

gregmedd commented Jul 8, 2024

gregmedd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmedd commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmedd commented Jul 25, 2024 •

edited

Loading